Skip to content

Enforce repair contract on the final repaired tree#341

Merged
steipete merged 7 commits into
openclaw:mainfrom
Jhacarreiro:fix/repair-contract-checkpoints-v2
Jul 4, 2026
Merged

Enforce repair contract on the final repaired tree#341
steipete merged 7 commits into
openclaw:mainfrom
Jhacarreiro:fix/repair-contract-checkpoints-v2

Conversation

@Jhacarreiro

@Jhacarreiro Jhacarreiro commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Repair artifacts can name files that must be present in the finished repair. The prior implementation enforced that contract at every intermediate checkpoint, so a valid initial repair could be rejected when a later review fix, base sync, or finalize step touched only an adjacent test or conflict file.

It could also publish recovery checkpoints before the whole repaired tree had been validated.

Why This Change Was Made

This keeps the contributor's canonical fix_artifact.repair_contract boundary, but makes the invariant describe the final product instead of each implementation step:

  • must_touch: required file or directory paths
  • match: any or all
  • no checkpoint-specific scope

The executor validates the final origin/<base>..HEAD changed-file set after review fixes, final base synchronization, and replacement-history compaction. That latest-base comparison prevents an upstream-only change from satisfying the contract.

Contract-bearing replacement jobs defer recovery pushes until this final validation passes. Jobs without a repair contract keep their existing checkpoint recovery behavior. Pure deterministic rebases cannot carry a repair contract.

User Impact

  • Valid repairs are no longer rejected because a later checkpoint touches a different file.
  • Invalid repairs fail closed before the finished replacement branch is published.
  • Maintainers get an error that names matched, missing, and final changed paths.
  • The implementation removes checkpoint phase state and the porcelain-status parser.

Evidence

Exact reviewed head: 97f4ff5ebeb3c225b55bac561716e76617484630

pnpm run check
  passed: build, lint, 597 unit tests, 595 repair tests, changed/full coverage, formatting

node --test test/repair/repair-contract.test.ts test/repair/execute-fix-artifact-source.test.ts test/repair/run-worker.test.ts test/repair/deterministic-automerge-result.test.ts test/repair/commit-finding-report.test.ts test/review-prompt-policy.test.ts
  passed (48/48)

codex exec --ephemeral --ignore-rules --sandbox read-only --output-schema schema/repair/codex-result.schema.json ...
  passed: a non-null fix artifact with repair_contract: null was accepted and emitted

The integration test creates a real temporary Git repository and proves both sides of the contract:

  1. A final branch that changes the required source path plus a later review-test path passes.
  2. A branch rebased over an upstream change to the required source path, while itself changing only docs, fails. The upstream-only change cannot satisfy the contract.

Fresh full-branch autoreview after the final implementation reported no accepted/actionable findings: patch is correct (0.82 confidence).

This is a maintainer rewrite of the contributor's original repair-contract proposal. The original commits and authors remain in the PR history.

@clawsweeper

clawsweeper Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 5:13 AM ET / 09:13 UTC.

Summary
Adds fix_artifact.repair_contract to the repair artifact contract, validates it at intake, enforces it once against the final latest-base branch delta, defers recovery pushes for contract-bearing jobs, and adds focused repair tests.

Reproducibility: yes. Current main has no repair_contract surface, and source inspection plus the added real-Git test show the intended failure mode: only the final latest-base branch delta should satisfy must_touch.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 12 files, +405/-6. The diff reaches schema, prompt, executor, validators, deterministic producers, and repair tests, so maintainers should review it as repair-lane automation work.
  • Strict schema guard: 1 recursive required-property test added. The added schema regression test directly targets the prior structured-output failure mode.

Root-cause cluster
Relationship: canonical
Canonical: #341
Summary: This PR is the active canonical repair-contract landing path; the earlier contract PR is closed unmerged, while the schema-outage and apply-queue PRs are adjacent automation work rather than the same root cause.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P2] This changes ClawSweeper repair automation by delaying recoverable branch pushes for contract-bearing jobs until final validation, so maintainers should treat exact-head CI and repair-lane review as merge gates.
  • [P1] The repair artifact schema gains a required repair_contract field with null as the compatibility value; model output is covered, but maintainers should be comfortable with that rollout for any external repair-artifact producers.

Maintainer options:

  1. Merge after exact-head automation checks (recommended)
    If current CI stays green, maintainers can merge while explicitly owning the repair-lane behavior change around final-only contract validation and recovery pushes.
  2. Narrow the rollout before merge
    If maintainers want a softer rollout, require a follow-up patch that keeps the schema field nullable but limits prompt emission or recovery-push deferral until more worker compatibility evidence exists.
  3. Pause if the artifact contract is not ready
    If the team does not want to add a canonical repair artifact field yet, pause or close this branch in favor of a smaller internal-only guard.

Next step before merge

  • No automated repair is needed; maintainers should review the repair-lane schema/executor rollout and merge only after exact-head checks are acceptable.

Security
Cleared: No concrete security or supply-chain regression was found; the diff adds no dependency or secret-permission expansion and validates contract paths before use.

Review details

Best possible solution:

Land this after exact-head CI and maintainer automation review confirm the schema rollout and recovery-push timing are acceptable, keeping repair_contract: null as the compatible default.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main has no repair_contract surface, and source inspection plus the added real-Git test show the intended failure mode: only the final latest-base branch delta should satisfy must_touch.

Is this the best way to solve the issue?

Yes. The current PR is a narrow maintainable path: it keeps the contract explicit, enforces it once on the final tree, makes null the compatible default, and adds regression coverage for the previous schema blocker.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 19ca145ed965.

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes exact-head terminal proof for pnpm run check, focused repair tests, and a local repair/Codex run showing the null and final-contract paths.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P1: The PR changes ClawSweeper repair/automerge safety behavior for final branch validation and recoverable checkpoint publishing.
  • merge-risk: 🚨 automation: Merging this changes repair-worker schema output and recovery-push timing, which could affect automated repair runs beyond what unit tests alone settle.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes exact-head terminal proof for pnpm run check, focused repair tests, and a local repair/Codex run showing the null and final-contract paths.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes exact-head terminal proof for pnpm run check, focused repair tests, and a local repair/Codex run showing the null and final-contract paths.
Evidence reviewed

What I checked:

  • Current main lacks the requested contract surface: A grep of current main found no repair_contract or must_touch implementation under src, schema, test, or prompts, so the central behavior is not already implemented on main. (19ca145ed965)
  • Strict schema compatibility blocker fixed: The latest head makes repair_contract a required fix-artifact property while allowing either null or the strict { must_touch, match } object, addressing the previous strict-schema incompatibility. (schema/repair/codex-result.schema.json:275, 97f4ff5ebeb3)
  • Final-tree enforcement is centralized: editValidatePrepareMerge detects contract-bearing artifacts, suppresses intermediate recovery pushes, calls final repair-contract enforcement after history compaction, and only then publishes a recoverable checkpoint. (src/repair/execute-fix-artifact.ts:2004, 97f4ff5ebeb3)
  • Latest-base diff is used for contract matching: The helper compares origin/${baseBranch}..HEAD with git diff --name-only -z, so an upstream-only base change cannot satisfy the repair contract. (src/repair/execute-fix-artifact.ts:3378, 97f4ff5ebeb3)
  • Contract validation rejects unsafe or incompatible shapes: validateRepairContractShape permits missing/null contracts, rejects extra keys and unsafe paths, requires match to be any or all, and disallows contracts on deterministic rebase-only artifacts. (src/repair/repair-contract.ts:56, 97f4ff5ebeb3)
  • Regression coverage exercises final-tree semantics: The added repair-contract test covers null contracts, shape validation, any/all and directory matching, cumulative checkpoint files, and a real Git scenario where upstream-only changes do not satisfy must_touch. (test/repair/repair-contract.test.ts:114, 97f4ff5ebeb3)

Likely related people:

  • steipete: Assigned on the PR and authored the latest maintainer rewrite commits that moved enforcement to the final repair tree and fixed the strict schema issue. (role: recent PR rewrite and reviewer; confidence: high; commits: bae964f9611a, aaf3304ddb58, 97f4ff5ebeb3; files: schema/repair/codex-result.schema.json, src/repair/execute-fix-artifact.ts, test/repair/repair-contract.test.ts)
  • vincentkoc: Recently merged repair-branch push safety work in src/repair/execute-fix-artifact.ts and also contributed intermediate commits on this repair-contract branch. (role: recent repair-lane contributor; confidence: medium; commits: ab37a53656df, 0c4af91947e5, 2e7164397659; files: src/repair/execute-fix-artifact.ts, test/repair/execute-fix-artifact-source.test.ts)
  • Jhacarreiro: Authored the original repair-contract branch commits and provided the local proof context that the maintainer rewrite preserves. (role: original branch implementer; confidence: medium; commits: d0e6569dc741, 00869acb9c7a; files: src/repair/execute-fix-artifact.ts, test/repair/repair-contract.test.ts)
  • Josh McKinney: Current-main blame for the central editValidatePrepareMerge repair flow traces to the grafted base commit, though the local history is shallow around that import. (role: base repair-flow author signal; confidence: low; commits: 471bf8065dd4; files: src/repair/execute-fix-artifact.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (2 earlier review cycles)
  • reviewed June 25, 2026, 10:38 AM ET / 14:38 UTC sha 486a5e0 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-04T07:59:52.687Z sha bae964f :: needs changes before merge. :: [P1] Keep repair_contract strict-schema compatible

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jun 19, 2026
@Jhacarreiro Jhacarreiro force-pushed the fix/repair-contract-checkpoints-v2 branch from 05484c7 to da96bec Compare June 20, 2026 07:42
@Jhacarreiro

Copy link
Copy Markdown
Contributor Author

Updated this PR after the maintainer deep review on #340.

Changes now address the two blockers called out there:

- no unsupported top-level must_touch / must_touch_files fields;
- canonical fix_artifact.repair_contract added to schema, prompt/output contract, validators and deterministic producers;
- executor now enforces fix_artifact.repair_contract only when present;
- parser now uses git status --porcelain=v1 -z --untracked-files=all;
- functional tests cover quoted/space paths, rename destinations, any/all semantics and likely_files bypass;
- local dry-run e2e proof rerun with repair_contract, allow -> planned, block -> rejected before checkpoint.

Validation run locally:

corepack pnpm run format:check
corepack pnpm run build:repair
node --test test/repair/repair-checkpoint-contract.test.ts test/repair/execute-fix-artifact-source.test.ts test/repair/deterministic-automerge-result.test.ts test/repair/commit-finding-report.test.ts
corepack pnpm run lint:repair
corepack pnpm run test:repair

All passed.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 20, 2026
@Jhacarreiro

Copy link
Copy Markdown
Contributor Author

v3 update pushed

I updated this PR to address the maintainer blockers from #340 instead of reopening or duplicating that closed PR.

What changed:

- `must_touch` / `must_touch_files` are no longer standalone unsupported fields.
- The contract is now canonical: `fix_artifact.repair_contract` in `schema/repair/codex-result.schema.json`.
- The prompt/output contract documents when to emit it and when to omit it.
- Runtime validation now checks the contract shape in execute-fix validation and review-results.
- Deterministic producers attach the contract only when the expected edit surface is concrete and bounded.
- The executor now reads `git status --porcelain=v1 -z --untracked-files=all`.
- Contract parsing/enforcement lives in `src/repair/repair-checkpoint-contract.ts` with functional tests.

Validation run locally:

corepack pnpm run format:check
corepack pnpm run build:repair
node --test test/repair/repair-checkpoint-contract.test.ts test/repair/execute-fix-artifact-source.test.ts test/repair/deterministic-automerge-result.test.ts test/repair/commit-finding-report.test.ts
corepack pnpm run lint:repair
corepack pnpm run test:repair

Result:

format:check: passed
build:repair: passed
focused tests: 19/19 passed
lint:repair: 0 warnings, 0 errors
test:repair: passed

Local dry-run executor proof with canonical fix_artifact.repair_contract:

allow:
  exit 0
  report_status planned
  action open_fix_pr planned
  checkpoint commit 9cdf11e6b647
  git_status clean

block:
  exit 1
  head remained c88270b81889
  working tree ?? docs/off-contract-proof.md
  error: repair checkpoint contract rejected initial: no required repair_contract.must_touch file changed; match=any; must_touch=docs/repair-contract-proof.md; changed_files=docs/off-contract-proof.md

@Jhacarreiro

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Please review the current head da96beca9e35f42da4d7da482bd622e6ba4909e7 and updated body/comment. This revision addresses the #340 maintainer blockers by moving the checkpoint contract into canonical fix_artifact.repair_contract, schema, prompt/output contract, validators, deterministic producers, and porcelain v1 z parsing.

@Jhacarreiro

Jhacarreiro commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Small implementation-shape note for review: if maintainers prefer a narrower contract activation policy, I can make that adjustment in this PR without changing the overall design.

The intentionally small knobs are:

- keep `repair_contract` schema/prompt/validator/executor semantics as the canonical boundary;
- make deterministic producers more conservative, or remove producer emission entirely if maintainers prefer model/planner-authored contracts only;
- make `match` / `scope` defaults explicit in runtime while preserving the schema contract, if that better matches existing artifact conventions.

One extra architectural motivation for this shape: keeping repair_contract inside the canonical fix_artifact contract gives external repair workers/connectors a stable boundary without adding another execution path inside ClawSweeper.

That keeps the core provider-neutral: ClawSweeper owns schema, validation, git/checkpoint safety, and PR mechanics, while any external worker that can emit the canonical artifact can interoperate through the same contract. This should make the repair lane easier to extend safely without growing backend-specific logic in the core.

Those would be narrow follow-ups on the current head, not a new PR and not a redesign of the checkpoint gate.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 21, 2026
@Jhacarreiro

Copy link
Copy Markdown
Contributor Author

Status refresh for maintainers:

  • branch is mergeable/CLEAN;
  • required checks are green;
  • no unresolved review threads are currently visible;
  • the latest revision keeps the checkpoint contract opt-in through fix_artifact.repair_contract rather than unsupported top-level fields.

Ready for maintainer review. If the preferred merge shape is narrower, I can adjust the activation policy without changing the core contract.

@steipete steipete self-assigned this Jul 4, 2026
@steipete steipete force-pushed the fix/repair-contract-checkpoints-v2 branch from 486a5e0 to bae964f Compare July 4, 2026 07:36
@steipete steipete changed the title Enforce repair contract at every checkpoint Enforce repair contract on the final repaired tree Jul 4, 2026
@steipete

steipete commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

@clawsweeper re-review

Please review exact head bae964f9611a3cf470158966e95a034f0ec7b30a. The maintainer rewrite now validates repair_contract once against the final latest-base branch delta, defers contract-bearing recovery pushes until that validation passes, and includes a real-git regression test showing that an upstream-only required-path change cannot satisfy the contract.

@clawsweeper

clawsweeper Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 4, 2026
@steipete

steipete commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

@clawsweeper re-review

The strict review schema outage is fixed on main by #404 (19ca145ed965294c71f1cfb1e1c7bcf912cd9877). Please review the exact current head bae964f9611a3cf470158966e95a034f0ec7b30a; this run is also the production E2E for the repaired schema and durable review-history path.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed P2 Normal priority bug or improvement with limited blast radius. labels Jul 4, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 4, 2026
@steipete steipete merged commit d3ad07b into openclaw:main Jul 4, 2026
11 checks passed
@steipete

steipete commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Landed in d3ad07b.

The final rewrite keeps the contributor's final-tree repair contract, makes repair_contract a required nullable strict-schema field, and aligns the deterministic automerge and commit-finding producers with that schema.

Proof on exact head 97f4ff5ebeb3c225b55bac561716e76617484630:

  • pnpm run check passed locally (597 unit tests, 595 repair tests, changed/full coverage, formatting).
  • Focused repair/schema suite passed 48/48.
  • A real codex exec --output-schema smoke accepted and emitted a non-null fix artifact with repair_contract: null.
  • Full-branch AutoReview found no actionable issues.
  • All required GitHub checks passed; pnpm check completed in 8m40s.

Thank you @Jhacarreiro for the original repair-contract proposal, and @vincentkoc for the rebase/checkpoint follow-up work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants